Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance warning about RPMs that were not validate by RPM #1459

Merged
merged 1 commit into from
May 28, 2024

Conversation

j-mracek
Copy link
Contributor

DNF5 informs about number of packages that signature was not verified, but without any additional detail. The ID of repository provides a good hint for user why the check was skipped.

Closes: #1311

Requires CI modification: rpm-software-management/ci-dnf-stack#1498

Copy link
Contributor

@pkratoch pkratoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. I have just a few minor notes that might be a matter of preference.

Also, the commit message says that "the ID of repository provides a good hint", but it doesn't say why. Can you please mention it's because the check is skipped per repo?

Comment on lines 1105 to 1107
_("Warning: skipped PGP checks for {0} package(s) from {1} repository(s)."),
num_checks_skipped,
repo_string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "(s)" for "repository" doesn't work as well as for "package". Firstly, it doesn't match the plural form, secondly, it's less clear when there is a list of repositories and not just a number. For example, this reads a bit weird: "skipped PGP checks for 82 package(s) from fedora, updates repository(s)." I would change it, so that the list of repos is at the end: "skipped PGP checks for 82 package(s) from repositories: fedora, updates". What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can even leverage plural forms here:
P_("skipped PGP checks for {} package from repository {}", "skipped PGP check for {} packages from repositories: {}", num_checks_skipped)
Or something similar, plurals translations are currently not used in dnf5 so this would be a pioneer :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about P_, but there are 3 possibility
"skipped PGP checks for {} package from repository {}"
"skipped PGP checks for {} packages from repository {}"
"skipped PGP checks for {} packages from repositories {}"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is not exactly a straightforward example for P_ as it contains two plurals. The only way would be splitting the message into two parts - one for package(s) and the other for repository(ies).
I'm not sure if this complication for translators is worth it, as in some languages it might be difficult to join such split sentences back together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils::sformat(
  P_(
    "skipped PGP checks for {} package from repositories: {}",
    "skipped PGP check for {} packages from repositories: {}",
     num_checks_skipped),
  num_checks_skipped
  repo_string);

@@ -1048,6 +1048,7 @@ bool Transaction::Impl::check_gpg_signatures() {
libdnf5::rpm::RpmSignature rpm_signature(base);
std::set<std::string> processed_repos{};
int num_checks_skipped = 0;
std::set<std::string> repo_of_checks_skipped;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this variable name. Wouldn't something like repos_with_skipped_checks be more intuitive?

@@ -1098,7 +1100,11 @@ bool Transaction::Impl::check_gpg_signatures() {
}
}
if (num_checks_skipped > 0) {
auto warning_msg = utils::sformat(_("Warning: skipped PGP checks for {} package(s)."), num_checks_skipped);
auto repo_string = libdnf5::utils::string::join(repo_of_checks_skipped, ", ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark ", " for translation. The joiner is language-specific.

@j-mracek
Copy link
Contributor Author

The PR and tests were updated according to suggestions.

DNF5 informs about number of packages that signature was not
verified, but without any additional detail. The ID of repository
provides a good hint for user why the check was skipped. The behavior
is related to configuration options which some of them  are repo
specific or specific for commandline repository. If user wants to verify
everything, the hint provides sufficient information which configuration
of repository should be modified.

Closes: rpm-software-management#1311
@pkratoch pkratoch added this pull request to the merge queue May 28, 2024
Merged via the queue into rpm-software-management:main with commit a889d09 May 28, 2024
11 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skipped PGP checks - but not sure which package or repo
4 participants